Flow rate initial support/v2 #12236
Flow rate initial support/v2 #12236
Conversation
This is to allow a way to match on the rate of the flow through rule language. This serves as a trivial first step to a more elaborate path to defining and detecting elephant flows. Feature 7438
Feature 7438
|
|
||
| Signature example:: | ||
|
|
||
| pass tcp any any -> any any (msg:"Flow rate higher than 50kbps"; flow.rate:>50kb; sid:1; rev:1;) |
There was a problem hiding this comment.
Not a fan of mismatching units. Considering keeping the initial syntax something like flow.rate:>500kb,10 where 500kb is the number of bytes and 10 the seconds with which the rate will be calculated.
There was a problem hiding this comment.
Some other ideas, none of which I'm a fan of:
flow.rate: 50kbps
flow.rate: 50kb,10 # document that first arg is bytes and second one the number of seconds
flow.rate: 50kb,10s
flow.rate: 50kb # document that this means 50kbytes per second
flow.rate: bytes 50kb, seconds 10 # inspired by threshold keyword
There was a problem hiding this comment.
Between those, I like:
flow.rate: 50kb,10s
The most, I think. I imagine that any format would need documenting.
There was a problem hiding this comment.
You may also want packets instead of bytes like flow.rate: packets 50, seconds 10
There was a problem hiding this comment.
Will you also want milliseconds ?
There was a problem hiding this comment.
Will you also want milliseconds ?
@catenacyber I don't know yet. Do you think somebody might want to express rate as 10k bytes in 5ms?
There was a problem hiding this comment.
I do not know either, but maybe more granularity/expressivity is always better ?
There was a problem hiding this comment.
Agreed. Shall keep that in mind. Thanks a lot for your valuable feedback, both! 🙇🏽♀️
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12236 +/- ##
=======================================
Coverage 83.18% 83.18%
=======================================
Files 912 913 +1
Lines 257169 257209 +40
=======================================
+ Hits 213914 213962 +48
+ Misses 43255 43247 -8
Flags with carried forward coverage won't be shown. Click here to find out more. |
| } | ||
|
|
||
| uint64_t age = SCTIME_SECS(p->flow->lastts) - SCTIME_SECS(p->flow->startts); | ||
| uint64_t rate = (p->flow->tosrcbytecnt + p->flow->todstbytecnt) / age; |
There was a problem hiding this comment.
Watch for division by zero ;-)
There was a problem hiding this comment.
indeed. an embarrassing mistake. thank you!
There was a problem hiding this comment.
This also shows lack of tests btw. Something like this should have easily been caught
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7438
SV_BRANCH=OISF/suricata-verify#2164
Previous PR: #12123
Changes since v1: